fix(render): scope strict lint to composition#2261
Conversation
|
Field feedback exposed a related no-index case: |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at ec4f456c.
Right scope for the fix. The trio of changes composes coherently: (1) target the explicit entry as the root HTML source, (2) skip compositions/ discovery so sibling entries aren't linted, (3) skip lintMultipleRootCompositions entirely since "multiple roots" is not a real problem when the caller explicitly named which root to use.
Traced the pieces that keep working correctly under the entry-scope path:
lintMissingOrEmptySubComposition(projectDir, rootHtml)atproject.ts:249walksdata-composition-srcrefs from the root HTML — and those refs are always root-relative (per the comment mirroringassertSubCompositionsUsable), so scoping the root tostandalone.htmlstill resolves its sub-compositions correctly againstprojectDir. No regression.collectExternalStyles(projectDir, rootHtml, rootCompSrcPath)— the newly-addedrootCompSrcPathcorrectly threads the entry's rel path intocollectLocalStylesheets, matching how nested sub-compositions already passcompSrcPathatproject.ts:233. Stylesheet resolution stays consistent between the two.- The
explicitEntry = args.composition && args.composition !== "." ? entryFile : undefinedguard in render.ts is correctly derived —entryFileis set fromresolveCompositionEntryArg(args.composition, ...)(statSync-validated) at line 396, so by the time we reach the lint gate the path is known-good. - Callers
lint.ts:47,preview.ts:215,publish.ts:44all still calllintProject(dir)positionally — the newentryFile?optional parameter is a pure additive extension, so those keep the old behavior.
Nits
-
🟡
rootFile = relative(resolve(projectDir), indexPath).replace(/\\/g, "/") || "index.html"atproject.ts:186— if the caller passes an entry file outsideprojectDir,relative()produces"../other/entry.html", which flows intoresults.push({ file: rootFile })and every downstream lint reporter as a path with parent traversal. No security issue (the file is still read fromindexPath, not the rel path), but the display + asset resolution would misbehave. The caller inrender.tswon't produce that shape today, but the function is now exported with a wider contract than that. Aif (!indexPath.startsWith(resolve(projectDir))) throw new Error(...)guard would fail fast for future callers. -
🟡 Test covers the passing case (clean project, entry with 0 findings). Would be tighter to also assert that when the entry file itself contains real findings (e.g. a broken asset reference in
standalone.html), those DO surface — proves the explicit-entry path is reporting entries, not just silently skipping the whole lint. One extra test case.
LGTM. Base of the stack — moving on to #2307 which builds on this.
Addressed in |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 reviewed at 89307271, delta from R1 head ec4f456c.
Both R1 nits cleanly addressed:
- Outside-project guard —
isWithinProjectRoot(projectDir, indexPath)gate atpackages/lint/src/project.ts:188reuses the same helper already used byresolveLocalAssetCandidates(line 142), so behavior is symmetric with the rest of the module.resolve(entryFile)without a base is safe here because the guard'srelative(projectRoot, candidate)+!startsWith("..") && !isAbsolute(relativePath)catches both absolute-outside and CWD-relative escapes. - Non-clean explicit-entry regression — new
it("reports findings from an explicit render composition entry", ...)assertstotalErrors > 0and matchesmedia_missing_idon the explicit entry. Companionit("rejects an explicit render composition entry outside the project", ...)locks the guard message shape.
LGTM from my side.
jrusso1020
left a comment
There was a problem hiding this comment.
Approving on Rames-D's go (R2 delta clean, nit round addressed) + Magi's CI-green confirmation. Verified at final head 89307271: CI green, no changes-requested, mergeable.
What
Scope pre-render lint to the file selected by
render --composition.Why
On CLI 0.7.53,
render --composition standalone.html --strictstill linted sibling root HTML files, emittedmultiple_root_compositions, and aborted. Users had to copy the same entry and assets into an isolated temporary project even though the command explicitly selected one composition.How
lintProjectto accept an explicit entry file.compositions/discovery and the project-wide multiple-root rule only for explicit-entry lint.--compositionis set.index.html.Test plan
bunx vitest run packages/cli/src/utils/lintProject.test.ts packages/cli/src/commands/render.test.ts(125 passed)bun run --filter @hyperframes/cli typechecknpx oxfmt --check packages/lint/src/project.ts packages/cli/src/commands/render.ts packages/cli/src/utils/lintProject.test.tsnpx oxlint packages/lint/src/project.ts packages/cli/src/commands/render.ts packages/cli/src/utils/lintProject.test.ts